Summary

This partially fixes #23248

There are are few circumstances where eslint-plugin-react-hooks will generate dependency arrays that unsafely access properties, even if those properties are safely accessed within the hook.

Problem 1: Preferring required over optional property access

Example 1:

useEffect(() => {
  if(props.foo?.bar) {
    console.log(props.foo.bar);
  }
}, [])

props.foo.bar is being accessed safely in the hook because of the if statement, but eslint-plugin-react-hooks will suggest a dependency array of [props.foo.bar] which will unsafely access the property resulting in an error like Uncaught TypeError: Cannot read properties of undefined (reading 'bar') when foo is undefined

Right now we always prefer treating a property as required if it appears both with and without optional chaining. In other words, in the example above props.foo.bar always wins over props.foo?.bar

The fix for this problem is mostly contained in the markNode function where optional access now takes precedence over required access.

Problem 2: Tracking the property as optional, not the parent object

Example 2:

useEffect(() => {
  if(props.foo?.bar) {
    console.log(props.foo.baz);
  }
}, [])

This hook safely accesses props.foo.baz (albeit in a strange way). In the current implementation we would generate a dependencies array of [props.foo?.bar, props.foo.baz], which doesn't safely access baz and could again lead to type errors coming from the dependencies array. The issue here is that we are tracking that props.foo.bar is optional when we should instead be tracking that the object, props.foo, should have all access done optionally.

The fix here is adjusting the optionalChains map to be a map of objects that have optional property access off of them, rather than a map of properties that are accessed optionally. This resulted in changes to markNode as well as formatDependency

For example 2, the optionalChains map before this change would look like:

{
  'props': false,
  'props.foo': false,
  'props.foo.bar': true,
  'props.foo.baz': false
}

and after this change it should look like:

{
  'props': false
  'props.foo': true
}

So before we would explicitly say that only props.foo.bar needed to be accessed optionally, but now we are saying that any property off of props.foo needs to be accessed optionally.

Problem 3: Not tracking optionalChains in declaredDependencies

In existing test cases like this we have a deps array like [ref1?.current, ref2?.current, props.someOtherRefs, props.color] and expect an error message containing React Hook useEffect has unnecessary dependencies: 'ref1.current' and 'ref2.current'. Notably the optional chaining is missing in the error message.

The fix here was to pass optionalChains in when calling analyzePropertyChain for a declared dependency node

Why I'm considering this a partial fix

We don't use optional chaining to determine if a dependency is missing or invalid, so this change will not force updates of existing dependency arrays or even alert you that there is potentially unsafe lack of optional chaining in an existing otherwise valid dependency array. I think this is the right tradeoff though, because it would be a pretty major and disruptive change to invalidate existing dependency arrays.

How did you test this change?

yarn test --prod
...
Test Suites: 322 passed, 322 total
Tests:       24 skipped, 9689 passed, 9713 total
Snapshots:   182 passed, 182 total
Time:        81.448 s, estimated 91 s
Ran all test suites.
✨  Done in 82.27s.

I added two new tests, matching example 1 and example 2 described above in this pull request. I also updated some existing tests to account for these changes. Specifically the pizza related tests are the most meaningfully changed

For example

useEffect(() => ({
  crust: pizza.crust,
  toppings: pizza?.toppings,
}), []);

before we would want a dependency array of [pizza.crust, pizza?.toppings], but now we are saying that properties on pizza must be accessed with optional chaining, so we would want [pizza?.crust, pizza?.toppings]